Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Workaround mypy issues with importlib.metadata #551

Merged
merged 5 commits into from
Dec 3, 2019

Conversation

bhrutledge
Copy link
Contributor

Closes #550

There were two errors on the failed build:

twine/__init__.py:24: error: Name 'importlib_metadata' already defined (by an import)
twine/__init__.py:27: error: Module has no attribute "metadata"

The first is an open issue on mypy: python/mypy#1153, and easily resolved with a # type: ignore on the except import. I don't know why we weren't getting that error before.

The second only happens in Python <3.8. I don't know why, since metadata does seem to be an attribute of both modules. Weirdly, removing the conditional import cleared the error.

I opted for the expedient solution of always using importlib_metadata, even though it's redundant in Python 3.8. If folks feel strongly, I can investigate further.

@bhrutledge bhrutledge requested a review from jaraco November 29, 2019 23:19
Copy link
Member

@jaraco jaraco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but I'm -1 on this change. It's recommended not to require importlib_metadata on Python 3.8, in part because there are outstanding issues, but also because I've encountered at least half a dozen other packages that are working to avoid using the third-party backport on Python 3.8, so this change runs counter to that intention. I'd started out not having a strong opinion on the matter and slightly preferring importing the backport on 3.8, but there's a clear preference out there for the split behavior.

More importantly, I feel like this workaround sidesteps the issue, degrades the intended behavior, and leaves other projects to run into the same issue. I think I'd rather have a solution that we would recommend to any project using importlib.metadata.

It seems a little bit of a mess that a linter (mypy) has made a very common pattern non-viable. Maybe mypy should support this use-case.

@bhrutledge bhrutledge changed the title Always use importlib_metadata to avoid mypy errors Workaround mypy issues with importlib.metadata Nov 30, 2019
@bhrutledge
Copy link
Contributor Author

@jaraco No worries. It was interesting to dig into. I made a repository to reproduce the errors and try out some fixes: https://github.com/bhrutledge/mypy-importlib-metadata. That's currently passing, but the commit history has more details. I thought it might be useful for reporting to mypy.

What do you think of this approach?

@bhrutledge bhrutledge requested a review from jaraco November 30, 2019 02:13
@bhrutledge
Copy link
Contributor Author

@jaraco I mentioned this on the related mypy issues, and got a helpful response: python/mypy#1393 (comment). This now uses sys.version_info to determine the proper import.

@jaraco
Copy link
Member

jaraco commented Dec 3, 2019

This approach is definitely viable. It still feels like this change has implications for the language as a whole, mainly that the Pythonic way of try/except for import is no longer viable. I'll follow up in the upstream issue. Thanks for tackling this issue.

@jaraco jaraco merged commit 95c9c91 into pypa:master Dec 3, 2019
musicinmybrain added a commit to musicinmybrain/build that referenced this pull request Jan 26, 2021
pypa/twine#551.

This fixes pypa#221, “Mypy error: Module 'importlib' has no attribute
'metadata'”.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spurious failure in CI linter
2 participants